Skip to content

feat: Bitwarden secret resolver and async register#398

Open
Coke1120 wants to merge 4 commits intoCortexReach:masterfrom
Coke1120:feat/bitwarden-secret-resolver
Open

feat: Bitwarden secret resolver and async register#398
Coke1120 wants to merge 4 commits intoCortexReach:masterfrom
Coke1120:feat/bitwarden-secret-resolver

Conversation

@Coke1120
Copy link
Copy Markdown

@Coke1120 Coke1120 commented Mar 29, 2026 •
edited
Loading

Summary

  • Add src/secret-resolver.ts supporting ${ENV_VAR} and bws://<secret-id> Bitwarden Secrets Manager refs for embedding, rerank, and LLM API keys
  • register() stays synchronous; async secret resolution runs in initPromise. Hooks await it before use; register() returns initPromise so callers that do await (tests, future host support) can wait for full init
  • service.start() logs a clear warn if secret resolution failed, so users know why memory is unavailable
  • bws access token passed via BWS_ACCESS_TOKEN env var instead of --access-token CLI arg to avoid process listing exposure
  • Skip double resolveEnvVars in embedder for already-resolved keys
  • Update openclaw.plugin.json docs to advertise bws:// support on all API key fields
  • Fix all tests to await plugin.register() and add selfImprovement: { enabled: false } to test configs that assert command:new === undefined

Test plan

  • node --test test/secret-resolver.test.mjs — verify Bitwarden and env-var resolution paths
  • npm test — full test suite passes

Split from #349.

🤖 Generated with Claude Code

@Coke1120 Coke1120 mentioned this pull request Mar 29, 2026
Closed
4 tasks
AliceLJY
AliceLJY approved these changes Mar 29, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation. execFile (not exec) avoids shell injection, execFileImpl injection enables proper unit testing, URL-based bws:// ref parsing is well-structured.

Note: PR #399 (Anthropic distill) depends on this PR — it includes all changes from this branch. Please merge #398 first, then #399.

@AliceLJY AliceLJY mentioned this pull request Mar 29, 2026
Open
4 tasks
@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Mar 30, 2026

Review: REQUEST-CHANGES

The Bitwarden secret resolver addresses a real operational gap (issue #349). Two issues need fixing before merge:

Must fix:

  1. Async register() breaks synchronous callers — The OpenClaw loader does not await the returned promise, but CLI/hook registration now happens only after await resolveSecretValues(). Hooks and tools are undefined until the promise settles. Test files at test/session-summary-before-reset.test.mjs:104 and :144 also have unawaited plugin.register(api) calls that observe partially initialized state.

  2. Test suite fails — selfImprovement: { enabled: false } was added to only one of three test config blocks in plugin-manifest-regression.mjs. The sessionDefaultApi and sessionEnabledApi fixtures still omit it, so command:new gets registered by the default-on selfImprovement feature and breaks the hook-absence assertions.

Worth considering (not blocking):

  • Resolved Bitwarden secrets are passed through resolveEnvVars() again in src/embedder.ts:422-424. If a real API key contains the literal substring ${...}, it will be re-interpreted as an env var template and throw. Bitwarden-resolved values should be treated as opaque.
  • The Bitwarden access token is passed as --access-token CLI argument to bws secret get, making it visible in process listings. Consider using an env var or stdin pipe instead.
  • Branch is behind main (stale_base=true) — please rebase.

AliceLJY
AliceLJY reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Bitwarden integration and the async register refactor — the architecture is sound overall. A few things to confirm before merging:

  1. Potentially stale test comment: test/session-summary-before-reset.test.mjs has a comment "register() is sync; before_reset is available immediately" — this seems to contradict the async change. Could you verify this test still passes and update the comment if it's outdated?

  2. Silent failure on initPromise rejection: When initFailed = true, all hooks silently skip without any warning to the user. For a secrets resolution failure this feels too quiet — would you consider at least logging a warn when a hook is skipped due to init failure, so users know why memory isn't working?

  3. Test checklist: test/secret-resolver.test.mjs and npm test are still unchecked — please confirm all tests pass before we merge.

Happy to approve once these are addressed!

Coke1120 and others added 4 commits March 31, 2026 18:24
@Coke1120 @claude
feat: add Bitwarden secret resolver and async register
03202ba 
Add src/secret-resolver.ts supporting ${ENV_VAR} and bws://<secret-id>
Bitwarden CLI secret references for embedding, rerank, and LLM API keys.
Make plugin register() async to support async secret resolution.
Update openclaw.plugin.json docs to advertise bws:// support.
Fix all tests to await plugin.register().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Coke1120 @claude
fix: disable selfImprovement in default manifest regression test config
0138b95 
With async register() now awaited, selfImprovement defaults to enabled
and registers command:new before the sessionMemory assertion runs.
Explicitly disable it in the base test config to isolate the assertion.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Coke1120 @claude
fix: revert register() to sync with ready-guard initPromise for secre…
65516b8 
…t resolution

Make register() synchronous again (OpenClaw loader does not await it).
All hook/tool registrations happen immediately; embedder, retriever, and
smartExtractor are initialized in a fire-and-forget initPromise that
resolves async secrets. Every hook that uses these awaits initPromise
before proceeding. register() returns initPromise so awaiting callers
(tests, host implementations that do support async) can still wait.

Also fix:
- bws access token passed via BWS_ACCESS_TOKEN env var instead of
  --access-token CLI arg to avoid exposure in process listings
- embedder double-resolveEnvVars: skip expansion if key lacks ${}
- selfImprovement: { enabled: false } in sessionDefaultApi,
  sessionEnabledApi, and session-summary harness to isolate assertions
  from the now-default-true selfImprovement feature

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Coke1120 @claude
fix: log warn on initFailed in service.start() and fix stale test com…
5fb9de7 
…ment

When secret resolution fails, service.start() now logs a visible warn
explaining why memory hooks are disabled, instead of silently skipping.

Update stale comment in session-summary test: register() returns
initPromise so awaiting it is meaningful, not just a sync guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Coke1120 Coke1120 force-pushed the feat/bitwarden-secret-resolver branch from fc782a2 to 5fb9de7 Compare March 31, 2026 10:30
@Coke1120 Coke1120 requested a review from AliceLJY March 31, 2026 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AliceLJY AliceLJY Awaiting requested review from AliceLJY

Assignees

@rwmjhb rwmjhb

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Coke1120 @rwmjhb @AliceLJY